Skip to content

Add support for disabling tradfri groups#7593

Merged
balloob merged 4 commits into
home-assistant:devfrom
cnrd:dev
May 17, 2017
Merged

Add support for disabling tradfri groups#7593
balloob merged 4 commits into
home-assistant:devfrom
cnrd:dev

Conversation

@cnrd
Copy link
Copy Markdown
Contributor

@cnrd cnrd commented May 14, 2017

Description:

Related issue (if applicable): fixes #

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#2632

Example entry for configuration.yaml (if applicable):

tradfri:
  allow_tradfri_groups: False

Checklist:

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies are only imported inside functions that use them (example).

@mention-bot
Copy link
Copy Markdown

@cnrd, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ggravlingen, @balloob and @fabaff to be potential reviewers.

@homeassistant
Copy link
Copy Markdown
Contributor

Hi @cnrd,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

Comment thread homeassistant/components/tradfri.py Outdated
return True

return (yield from _setup_gateway(hass, config, host, key))
return (yield from _setup_gateway(hass, config, host, key, allow_tradfri_groups))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line too long (85 > 79 characters)

Comment thread homeassistant/components/tradfri.py Outdated

if host in keys:
yield from _setup_gateway(hass, config, host, keys[host]['key'])
yield from _setup_gateway(hass, config, host, keys[host]['key'], allow_tradfri_groups)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line too long (98 > 79 characters)

Comment thread homeassistant/components/tradfri.py Outdated
vol.Inclusive(CONF_HOST, 'gateway'): cv.string,
vol.Inclusive(CONF_API_KEY, 'gateway'): cv.string,
vol.Optional(CONF_ALLOW_TRADFRI_GROUPS,
default=DEFAULT_ALLOW_TRADFRI_GROUPS): cv.boolean,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

continuation line under-indented for visual indent

@MartinHjelmare
Copy link
Copy Markdown
Member

MartinHjelmare commented May 14, 2017

What's the usecase of this addition?

@cnrd
Copy link
Copy Markdown
Contributor Author

cnrd commented May 14, 2017

Same usecase as in the Hue Component (https://home-assistant.io/components/light.hue/):
allow_hue_groups (Optional): (true/false) Enable this to stop Home Assistant from importing the groups defined on the Hue bridge.

The documentation should probably just be:
allow_tradfri_groups (Optional): (true/false) Enable this to stop Home Assistant from importing the groups defined on the Tradfri bridge.

@MartinHjelmare
Copy link
Copy Markdown
Member

But what's the benefit of not adding the group as an entity?

@cnrd
Copy link
Copy Markdown
Contributor Author

cnrd commented May 14, 2017

Not having to deal with Emulated Hue and/or Homebridge automatically importing them.

Yes I know it is possible to disable this on the individual groups, by adding them in customization.yaml, but I would rather they did not get added as entities at all.
The way I see bridges are just that, bridges that allows me to talk directly to what is connected to them. The bridge should not add any extra stuff by itself.

I'm not completely sure, but I think that it is not required to add groups in Hue, but in Tradfri it is required to have at least one group, that is used to add other devices.

That is my usecase for disabling the groups, but I know that some people may want to use them, which is why the default is that groups are added as entities, this change just gives people the option to actively disable them.

@Shwamp
Copy link
Copy Markdown

Shwamp commented May 15, 2017

Hey guys, a HA beginner here.. Got to to agree that the groups are kind off unnecessary. Got about 15 units and the only reason for me to have groups is in the setup of TRÅDFRI gateway for the first time. Sins it is a needed to pair with the remotes with the lamps.

Copy link
Copy Markdown
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good addition and the code looks 👍 🐬

@balloob
Copy link
Copy Markdown
Member

balloob commented May 17, 2017

@MartinHjelmare I think the addition makes sense. That way there are less entities to fetch data from.

@balloob balloob merged commit d2ed3a1 into home-assistant:dev May 17, 2017
@home-assistant home-assistant locked and limited conversation to collaborators Sep 4, 2017
@ghost ghost removed the platform: light.tradfri label Mar 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants